Conversation
seyfeb
force-pushed
the
maintenance/removeJQuery
branch
from
a4ddaa0 to
7d6ec6c
Compare
seyfeb
force-pushed
the
maintenance/removeJQuery
branch
from
fa28721 to
ccd0695
Compare
|
As far as I see the tests are running well on your fork. If this is true, you might well mark this PR as ready for review (hopefully triggering the tasts here as well). |
seyfeb
force-pushed
the
maintenance/removeJQuery
branch
from
0c1be68 to
1476b9a
Compare
Codecov Report
@@ Coverage Diff @@
## master #497 +/- ##
========================================
Coverage 0.87% 0.87%
Complexity 436 436
========================================
Files 14 14
Lines 1375 1375
========================================
Hits 12 12
Misses 1363 1363
Flags with carried forward coverage won't be shown. Click here to find out more. |
christianlupus
left a comment
christianlupus
left a comment
There was a problem hiding this comment.
I did not test all cases (especially not all failure cases) as it is quite cumbersome to do so. Looks good in general, a few changes I marked (some are only comments of course).
| if ($this.$store.state.recipe.recipeInstructions) { | ||
| $this.instructions = Object.values($this.$store.state.recipe.recipeInstructions) | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.keywords) { | ||
| $this.keywords = String($this.$store.state.recipe.keywords).split(',') | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.cookTime) { | ||
| let cookT = $this.$store.state.recipe.cookTime.match(/PT(\d+?)H(\d+?)M/) | ||
| $this.timerCook = { hours: parseInt(cookT[1]), minutes: parseInt(cookT[2]) } | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.prepTime) { | ||
| let prepT = $this.$store.state.recipe.prepTime.match(/PT(\d+?)H(\d+?)M/) | ||
| $this.timerPrep = { hours: parseInt(prepT[1]), minutes: parseInt(prepT[2]) } | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.totalTime) { | ||
| let totalT = $this.$store.state.recipe.totalTime.match(/PT(\d+?)H(\d+?)M/) | ||
| $this.timerTotal = { hours: parseInt(totalT[1]), minutes: parseInt(totalT[2]) } | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.tool) { | ||
| $this.tools = $this.$store.state.recipe.tool | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.dateCreated) { | ||
| let date = $this.parseDateTime($this.$store.state.recipe.dateCreated) | ||
| $this.dateCreated = (date != null ? date.format('L, LT').toString() : null) | ||
| } | ||
|
|
||
| if ($this.$store.state.recipe.dateModified) { | ||
| let date = $this.parseDateTime($this.$store.state.recipe.dateModified) | ||
| $this.dateModified = (date != null ? date.format('L, LT').toString() : null) | ||
| } | ||
| if ($this.$store.state.recipe.nutrition) { | ||
| if ( $this.$store.state.recipe.nutrition instanceof Array) { | ||
| $this.$store.state.recipe.nutrition = {} | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This might be well suited for a computed property from the recipe data itself.
There was a problem hiding this comment.
yes. what do you think is better: a recipe object as a computed property or a computed property for each recipe part (ingredients, dateModified, etc.)?
There was a problem hiding this comment.
I assume both are similarly useful. Personally, I assume a single property with an object might be simpler to use if we think what might (or might not) be able to extend/abstract the component.
|
|
||
| // Start the app once document is done loading | ||
| $(document).ready(function () { | ||
| document.addEventListener("DOMContentLoaded", function(event) { |
There was a problem hiding this comment.
I am unsure if this really needed. It will most probably not cause issues but I found nowhere the requirement documented.
There was a problem hiding this comment.
Me neither. We could remove this and try if it still works
There was a problem hiding this comment.
Let's see what comes out from my question. I fear it will be more of unstable behavior and less of no function at all.
There was a problem hiding this comment.
That’s what I would expect, too. Good idea to ask in the vue forum!
There was a problem hiding this comment.
I already asked in the Discourse app but there was only an uncertain statement given.
There was a problem hiding this comment.
funny how nobody seems to know
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
…n in app controls Signed-off-by: Sebastian Fey <info@sebastianfey.de>
…n in app index and app navi Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
…dit input group. Fixed pasting content without newline in input group Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Added @nextcloud/auth Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
christianlupus
force-pushed
the
maintenance/removeJQuery
branch
from
0cfb693 to
57045ea
Compare
2 participants
As NC is dropping the support for the global jQuery and including the library within our app seems to be a bit of an overkill, I replaced the jQuery ajax calls by @nextcloud/axios and switched to plain JS in some places.
Closes #475